Fix IOU Options bug#8839
Conversation
Show Request and Send Money options when there are two participants, Splitbill option when there are more than two participants and no option if there is only one participant
melvin-bot
Bot
requested review from
Santhosh-Sellavel and
iwiznia
and removed request for
a team
Santhosh-Sellavel
left a comment
Santhosh-Sellavel
left a comment
There was a problem hiding this comment.
Can you check out this #8357 (comment)
I've asked to move the logic to a new method!
Done. @iwiznia @Santhosh-Sellavel I have found another existing bug. While splitting bill, the logged in user(Chaudary Dai in this case) is counted twice. I think changing to in IOUModal.js solves the issue. Should I go through whole process of reporting? Or will I be compensated if I solved it here? |
| /** | ||
| * Returns the list of report compose actions | ||
| * | ||
| * @param {Boolean} showSplitBill | ||
| * @param {Boolean} showSendRequestMoney | ||
| * @param {Boolean} hasExcludedIOUEmails | ||
| * @param {Function} openPicker | ||
| * @param {Function} displayFileInModal | ||
| * @returns {Array<object>} | ||
| */ | ||
| getReportComposeActions(showSplitBill, showSendRequestMoney, hasExcludedIOUEmails, openPicker, displayFileInModal) { | ||
| return [ |
There was a problem hiding this comment.
Do we really need to compute and pass everything to this function it does not make any sense?
| ...(!hasExcludedIOUEmails | ||
| && Permissions.canUseIOU(this.props.betas) | ||
| && showSplitBill ? [ | ||
| { | ||
| icon: Expensicons.Receipt, | ||
| text: this.props.translate('iou.splitBill'), | ||
| onSelected: () => { | ||
| Navigation.navigate( | ||
| ROUTES.getIouSplitRoute( | ||
| this.props.reportID, | ||
| ), | ||
| ); | ||
| }, | ||
| }, | ||
| ] : []), | ||
| ...(!hasExcludedIOUEmails | ||
| && Permissions.canUseIOU(this.props.betas) | ||
| && showSendRequestMoney ? [ | ||
| { | ||
| icon: Expensicons.MoneyCircle, | ||
| text: this.props.translate('iou.requestMoney'), | ||
| onSelected: () => { | ||
| Navigation.navigate( | ||
| ROUTES.getIouRequestRoute( | ||
| this.props.reportID, | ||
| ), | ||
| ); | ||
| }, | ||
| }, | ||
| ] : []), | ||
| ...(!hasExcludedIOUEmails && Permissions.canUseIOUSend(this.props.betas) && showSendRequestMoney ? [ | ||
| { | ||
| icon: Expensicons.Send, | ||
| text: this.props.translate('iou.sendMoney'), | ||
| onSelected: () => { | ||
| Navigation.navigate( | ||
| ROUTES.getIOUSendRoute( | ||
| this.props.reportID, | ||
| ), | ||
| ); | ||
| }, | ||
| }, | ||
| ] : []), | ||
| { | ||
| icon: Expensicons.Paperclip, | ||
| text: this.props.translate('reportActionCompose.addAttachment'), | ||
| onSelected: () => { | ||
| openPicker({ | ||
| onPicked: (file) => { | ||
| displayFileInModal({file}); | ||
| }, | ||
| }); | ||
| }, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Code is not DRY, we have redundant checks here. Can you refactor this?
There was a problem hiding this comment.
Refactoring this might result into nested ternary operators. I was discouraged to use nested ternary operators in previous PRs. Any suggestion?
There was a problem hiding this comment.
We could use simple if/else no need for ternary here.
There was a problem hiding this comment.
It will look something like this. Any suggestion?
const actions = [];
if (!hasExcludedIOUEmails) {
if (Permissions.canUseIOU(this.props.betas)) {
if (showSplitBill) {
actions.push(
{
icon: Expensicons.Receipt,
text: this.props.translate('iou.splitBill'),
onSelected: () => {
Navigation.navigate(
ROUTES.getIouSplitRoute(
this.props.reportID,
),
);
},
},
);
}
if (showSendRequestMoney) {
actions.push({
icon: Expensicons.MoneyCircle,
text: this.props.translate('iou.requestMoney'),
onSelected: () => {
Navigation.navigate(
ROUTES.getIouRequestRoute(
this.props.reportID,
),
);
},
});
}
}
if (Permissions.canUseIOUSend(this.props.betas) && showSendRequestMoney) {
actions.push({
icon: Expensicons.Send,
text: this.props.translate('iou.sendMoney'),
onSelected: () => {
Navigation.navigate(
ROUTES.getIOUSendRoute(
this.props.reportID,
),
);
},
});
}
}
actions.push({
icon: Expensicons.Paperclip,
text: this.props.translate('reportActionCompose.addAttachment'),
onSelected: () => {
openPicker({
onPicked: (file) => {
displayFileInModal({file});
},
});
},
});
return actions;
There was a problem hiding this comment.
Let's just move IOU options computation alone to a new method, leave Add Attachment untouched thanks!
getIOUOptions(reportParticipants) {
const iouOptions = [];
const participants = _.filter(reportParticipants, email => this.props.myPersonalDetails.login !== email).length > 1;
const hasExcludedIOUEmails = lodashIntersection(reportParticipants, CONST.EXPENSIFY_EMAILS).length > 0;
const hasMultipleParticipants = participants.length > 1;
if (hasExcludedIOUEmails || participants.length === 0 || Permissions.canUseIOU(this.props.betas)) {
return iouOptions;
}
if (hasMultipleParticipants) {
// append split
}
else {
if (Permissions.canUseIOUSend(this.props.betas)) {
// append send
}
// append request
}
return iouOptions;
}cc: @iwiznia
| participants={this.props.hasMultipleParticipants | ||
| ? this.state.participants | ||
| : _.filter(this.state.participants, email => this.props.myPersonalDetails.login !== email.login)} |
There was a problem hiding this comment.
Should this be done here, I think it could be moved along the lines here
Lines 110 to 111 in 4c64bec
There was a problem hiding this comment.
Participants are updated during IOUParicipantsPage step. So, it cannot be moved here. We need to make use of updated participants list.
There was a problem hiding this comment.
IOUParicipantsPage will not be used for Room or group flow. So no issues there.
There was a problem hiding this comment.
Check and let me know!
There was a problem hiding this comment.
I don't think it's a good idea. It adds more complication.
|
@sobitneupane I believe this is well within the scope of this issue. cc @iwiznia |
I have already solved the issue mentioned in this comment while solving this issue as it is part of this issue. It would have been an issue introduced while solving this issue. But while testing I found that the issue I mentioned also occurs. If it is believed to be part of this issue I will solve it here. It was an existing issue. |
Thanks, I would let @iwiznia take the final call! |
Solved this issue as well. I am okay either way. |
|
I think it's basically the same issue or very closely related, so original amount should apply. If you disagree, please let me know and we can discuss. |
| onSelected: () => { | ||
| Navigation.navigate( | ||
| ROUTES.getIOUSendRoute( | ||
| this.props.reportID, | ||
| ), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
Given it's only one param per method and line is not too long, I think this is more readable
| onSelected: () => { | |
| Navigation.navigate( | |
| ROUTES.getIOUSendRoute( | |
| this.props.reportID, | |
| ), | |
| ); | |
| }, | |
| onSelected: () => Navigation.navigate(ROUTES.getIOUSendRoute(this.props.reportID)), |
There was a problem hiding this comment.
made the requested change
| * @returns {Array<object>} | ||
| */ | ||
| getIOUOptions(reportParticipants) { | ||
| const iouOptions = []; |
There was a problem hiding this comment.
Remove this (see other comments)
There was a problem hiding this comment.
made the requested change
| const hasMultipleParticipants = participants.length > 1; | ||
|
|
||
| if (hasExcludedIOUEmails || participants.length === 0 || !Permissions.canUseIOU(this.props.betas)) { | ||
| return iouOptions; |
There was a problem hiding this comment.
| return iouOptions; | |
| return []; |
There was a problem hiding this comment.
made the requested change
| iouOptions.push( | ||
| { | ||
| icon: Expensicons.Receipt, | ||
| text: this.props.translate('iou.splitBill'), | ||
| onSelected: () => { | ||
| Navigation.navigate( | ||
| ROUTES.getIouSplitRoute( | ||
| this.props.reportID, | ||
| ), | ||
| ); | ||
| }, | ||
| }, | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
Early return here
| iouOptions.push( | |
| { | |
| icon: Expensicons.Receipt, | |
| text: this.props.translate('iou.splitBill'), | |
| onSelected: () => { | |
| Navigation.navigate( | |
| ROUTES.getIouSplitRoute( | |
| this.props.reportID, | |
| ), | |
| ); | |
| }, | |
| }, | |
| ); | |
| } else { | |
| return [{ | |
| icon: Expensicons.Receipt, | |
| text: this.props.translate('iou.splitBill'), | |
| onSelected: () => Navigation.navigate(ROUTES.getIouSplitRoute(this.props.reportID)), | |
| }]; | |
| } |
There was a problem hiding this comment.
made the requested change
| }, | ||
| ); | ||
| } else { | ||
| iouOptions.push({ |
There was a problem hiding this comment.
| iouOptions.push({ | |
| const iouOptions = []; | |
| iouOptions.push({ |
There was a problem hiding this comment.
else statement is removed as it is no longer needed and variable is declared at the top of the function.
| onSelected: () => { | ||
| Navigation.navigate( | ||
| ROUTES.getIouRequestRoute( | ||
| this.props.reportID, | ||
| ), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
| onSelected: () => { | |
| Navigation.navigate( | |
| ROUTES.getIouRequestRoute( | |
| this.props.reportID, | |
| ), | |
| ); | |
| }, | |
| onSelected: () => Navigation.navigate(ROUTES.getIOUSendRoute(this.props.reportID)), |
There was a problem hiding this comment.
made the requested change
|
@Santhosh-Sellavel if everything looks good, please approve the PR. |
| myPersonalDetails: { | ||
| key: ONYXKEYS.MY_PERSONAL_DETAILS, | ||
| }, |
There was a problem hiding this comment.
prop type is defined for myPersonalDetails can you add that, thanks!
PR Reviewer Checklist
|
|
@sobitneupane Example: |
|
Waiting till the comment above is addressed before merging |
Added. |
|
@iwiznia We are good to go here! |
|
🚀 Cherry-picked to staging by @sketchydroide in version: 1.1.57-8 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀
|
4 participants
Show Request and Send Money options when there are two participants, Splitbill option when there are more than two participants and no option if there is only one participant
Details
Fixed Issues
$ #8357
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS
Android